Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Put all GPIO defines in gpio_bitaxe.h #566

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Put all GPIO defines in gpio_bitaxe.h #566

wants to merge 3 commits into from
+56 −179

Conversation

mutatrum
Copy link
Contributor

@mutatrum mutatrum commented Dec 8, 2024

Fixes #547

mutatrum
mutatrum commented Dec 8, 2024
View reviewed changes
main/led_controller.c
.channel = LEDC_CHANNEL_3,
.timer_sel = LEDC_TIMER_0,
.intr_type = LEDC_INTR_DISABLE,
.gpio_num = 38, // Define the output GPIO
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pins 38, 39 and 40 were not configured, so I figured this file was both unused and broken.

@mutatrum
Copy link
Contributor Author

mutatrum commented Dec 9, 2024

Better suggestion for the filename are welcome. Naming things is still one of the hardest problems in CS.

@WantClue WantClue added enhancement New feature or request accepted This issue will be worked on labels Dec 10, 2024
@WantClue WantClue self-assigned this Dec 10, 2024
WantClue
WantClue approved these changes Dec 10, 2024
View reviewed changes
Copy link
Collaborator

@WantClue WantClue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will wait for LVGL before we move ahead.

@WantClue
Copy link
Collaborator

please resolve conflicts

@mutatrum
Copy link
Contributor Author

Conflicts resolved

eandersson
eandersson reviewed Dec 13, 2024
View reviewed changes
main/gpio_bitaxe.h
@@ -0,0 +1,11 @@
#ifndef GPIO_BITAXE_H_
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only comment is that it feels backwards that components would depend on a header in the app (main).

@mutatrum
Copy link
Contributor Author

One option would be to add the pin config to Kconfig.projbuild:

    menu "GPIO Pin Configuration"

        config GPIO_BUTTON_BOOT
            int "Boot button GPIO pin"
            default 0
            help
                GPIO pin connected to the LED.

        config GPIO_ASIC_RESET
            int "ASIC reset GPIO pin"
            default 1
            help
                GPIO pin connected to the Button.

        config GPIO_ASIC_ENABLE
            int "ASIC enable GPIO pin"
            default 10
            help
                GPIO pin for I2C data line (SDA).

        config GPIO_PLUG_SENSE
            int "Barrel plug sense GPIO pin"
            default 12
            help
                GPIO pin for I2C clock line (SCL).

        config GPIO_I2C_SDA
            int "I2C SDA Pin"
            default 47
            help
                GPIO pin for I2C clock line (SCL).
            
        config GPIO_I2C_SCL
            int "I2C SCL Pin"
            default 48
            help
                GPIO pin for I2C clock line (SCL).
            
    endmenu

This would eliminate gpio_bitaxe.h completely and remove all the imports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eandersson eandersson eandersson left review comments

@WantClue WantClue WantClue approved these changes

Assignees

@WantClue WantClue

Labels
accepted This issue will be worked on enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Chore: define all GPIO pins in a single place
3 participants
@mutatrum @WantClue @eandersson